Roles dynamically added with ngAria #10318

marcysutton · 2014-12-04T00:41:00Z

To communicate the purpose of custom inputs with a type of range, checkbox or radio, ngAria now dynamically adds roles to custom radio buttons, checkboxes and sliders.

It also adds a role of button to any element with ngClick that is not a button, anchor, input or textarea (such as a div). This feature comes on the heels of #10288, which binds the keypress event for ngClick. With these two changes together, users of assistive technologies will know what they are focused on and be able to operate controls with the keyboard (this is important because ngAria dynamically adds tabIndex="0" with ngClick). However, not all uses of ngClick are used for buttons–sometimes developers bind ng-click on the document. In this scenario, a role can be overridden by the developer if button does not match expected behavior.

Note: This change does not implement any kind of flag allowing the user to opt out of roles being set.

Closes #9254 and #10012.

googlebot · 2014-12-04T00:41:01Z

CLAs look good, thanks!

gkalpak · 2014-12-04T06:34:34Z

I know it is unrelated to this PR, but I was wondering (with regard to line 209):

((type || role) === 'checkbox' ...

What if the element has both a type and a role (and they are different) ?
Is it even a "legitimate" use-case ?

marcysutton · 2014-12-04T19:32:12Z

@gkalpak not for checkbox. <input type="range"> and role="slider" is a use case, and there may be others....but they'd have to be looked at on a case by case basis. I'll keep it in mind though.

gkalpak · 2014-12-05T10:34:51Z

@marcysutton: Aha, thx. So, if I interpret your answer correctly, in the "general" case each attribute (type, role) should be checked separately:

((type === 'xyz') || (role === 'xyz') || ...)

But for checkboxes and radios, type and role (if set) will be the same, so it is OK to write:

((type || role) === 'xyz')

(which will essentially ignore role if type is defined, but this is OK because role shouldn't be checkbox/radio unless type is checkbox/radio as well.)

Is my thinking correct ?

caitp · 2014-12-05T19:00:13Z

test/ngAria/ariaSpec.js

+      compileInput('<div ng-click="someFunction()"></div>');
+      expect(element.attr('role')).toBe('button');
+    });
+    it('should not add role="button" to anchor', function() {


nit: 2 empty lines between each suite/spec/block

caitp · 2014-12-05T19:05:25Z

src/ngAria/aria.js

@@ -319,6 +329,10 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) {
          }
        });
      }
+
+      if (!elem.attr('role') && (elem[0].nodeName !== 'BUTTON') && (elem[0].nodeName !== 'A')) {


I wonder if it would be more readable to have something like && !IsNodeOneOf(elem, "BUTTON", "A") or something. I don't think it's problematic like this, but if there was one extra condition added it would be really hard to read.

Great point. That conditional is a little awkward in its current form.

marcysutton · 2014-12-05T22:55:11Z

@gkalpak there is no "generic" case. ngAria is checking for specific scenarios to add the right attributes. type || role applies correctly to each case where it is being used.

gkalpak · 2014-12-05T23:46:39Z

OK, thx ! I get it (I think :))

dylanb · 2015-02-07T22:44:35Z

src/ngAria/aria.js

@@ -310,6 +320,9 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) {
            return true;
          }
        }
+        if (!elem.attr('role') && !isNodeOneOf(elem, ['BUTTON', 'A'])) {


blacklist should include INPUT and TEXTAREA

Good call. But do people put ng-click on those? shudder

input type=image, type=submit, type=button will all commonly have ng-click attributes applied

Great point. I updated the blacklist and added it to the dynamic keypress binding so there would be more consistency.

marcysutton · 2015-02-07T23:03:00Z

@caitp I rebased this branch with the recently merged ngAria changes. The code now uses a node blacklist to add roles and bind keypress events.

marcysutton · 2015-02-16T18:44:52Z

@Narretz @petebacondarwin can you take a look at this PR for adding roles dynamically? It's the last ngAria change that would be great to have in the 1.4 release. Thank you!

gkalpak · 2015-02-16T19:16:10Z

test/ngAria/ariaSpec.js

+    });
+
+    it('should add missing role="checkbox" to custom input', function() {
+      scope.$apply('val = true');


What is the purpose of this line ?

It was unnecessary. Cleaned up!

Adds missing roles: slider, radio, checkbox Closes angular#10012

Closes angular#9254

petebacondarwin · 2015-02-18T13:49:26Z

LGTM

andrewaustin · 2015-02-24T20:24:16Z

Would love to see this in 1.3.x. We have some tags that are clickable and are currently having to do some extra work with ng-keypress to make them fully accessible. Having a blacklist rather than the current div & li whitelist would be extremely useful.

ginader · 2015-02-27T23:58:26Z

👍

Closes #9254 Closes #10318

This change adds the missing roles: `slider`, `radio`, `checkbox` Closes #10012 Closes #10318

Closes #9254 Closes #10318

petebacondarwin · 2015-03-02T13:50:07Z

Landed in master and cherry picked to 1.3.x for good measure.

marcysutton · 2015-03-02T22:44:33Z

Woohoo! Thanks @petebacondarwin! 🎉

This change adds the missing roles: `slider`, `radio`, `checkbox` Closes angular#10012 Closes angular#10318

Closes angular#9254 Closes angular#10318

This change adds the missing roles: `slider`, `radio`, `checkbox` Closes angular#10012 Closes angular#10318

Closes angular#9254 Closes angular#10318

googlebot added the cla: yes label Dec 4, 2014

marcysutton force-pushed the ngaria-roles branch from b420186 to 90eb3a9 Compare December 4, 2014 00:47

caitp reviewed Dec 5, 2014
View reviewed changes

caitp added component: ngAria type: feature labels Dec 5, 2014

caitp added this to the 1.3.7 milestone Dec 5, 2014

caitp reviewed Dec 5, 2014
View reviewed changes

marcysutton force-pushed the ngaria-roles branch from 90eb3a9 to 91776c2 Compare December 5, 2014 22:40

caitp modified the milestones: 1.3.7, 1.3.8 Dec 9, 2014

pkozlowski-opensource modified the milestones: 1.3.8, 1.3.7 Dec 9, 2014

petebacondarwin modified the milestones: 1.3.7, 1.3.8 Dec 15, 2014

btford modified the milestones: 1.3.8, 1.3.9 Dec 19, 2014

lgalfaso removed this from the 1.4.0-beta.2 / 1.3.11 milestone Feb 5, 2015

marcysutton force-pushed the ngaria-roles branch 2 times, most recently from cb6e000 to 0cdd758 Compare February 7, 2015 22:33

dylanb reviewed Feb 7, 2015
View reviewed changes

marcysutton force-pushed the ngaria-roles branch from 0cdd758 to 9133e14 Compare February 7, 2015 22:59

marcysutton force-pushed the ngaria-roles branch from 9133e14 to 708eacb Compare February 7, 2015 23:04

gkalpak reviewed Feb 16, 2015
View reviewed changes

Marcy Sutton added 2 commits February 17, 2015 11:11

feat(ngAria): Roles added to custom inputs

3894b95

Adds missing roles: slider, radio, checkbox Closes angular#10012

feat(ngAria): Adds button role to ng-click

7fafdf9

Closes angular#9254

marcysutton force-pushed the ngaria-roles branch from 708eacb to 7fafdf9 Compare February 17, 2015 19:14

petebacondarwin modified the milestones: 1.4.0-beta.5 / 1.3.14, 1.4.0-beta.6 / 1.3.15 Feb 24, 2015

petebacondarwin closed this in 29cdaee Mar 2, 2015

petebacondarwin pushed a commit that referenced this pull request Mar 2, 2015

feat(ngAria): add button role to ngClick

bb36507

Closes #9254 Closes #10318

petebacondarwin pushed a commit that referenced this pull request Mar 2, 2015

feat(ngAria): add roles to custom inputs

2136994

This change adds the missing roles: `slider`, `radio`, `checkbox` Closes #10012 Closes #10318

petebacondarwin pushed a commit that referenced this pull request Mar 2, 2015

feat(ngAria): add button role to ngClick

b9ad91c

Closes #9254 Closes #10318

hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015

feat(ngAria): add roles to custom inputs

cfefbc0

This change adds the missing roles: `slider`, `radio`, `checkbox` Closes angular#10012 Closes angular#10318

hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015

feat(ngAria): add button role to ngClick

e4dd239

Closes angular#9254 Closes angular#10318

marcysutton deleted the ngaria-roles branch April 11, 2015 21:14

netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

feat(ngAria): add roles to custom inputs

b4ddeb0

This change adds the missing roles: `slider`, `radio`, `checkbox` Closes angular#10012 Closes angular#10318

netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

feat(ngAria): add button role to ngClick

a3a8bc8

Closes angular#9254 Closes angular#10318

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roles dynamically added with ngAria #10318

Roles dynamically added with ngAria #10318

marcysutton commented Dec 4, 2014

googlebot commented Dec 4, 2014

gkalpak commented Dec 4, 2014

marcysutton commented Dec 4, 2014

gkalpak commented Dec 5, 2014

caitp Dec 5, 2014

caitp Dec 5, 2014

marcysutton Dec 5, 2014

marcysutton commented Dec 5, 2014

gkalpak commented Dec 5, 2014

dylanb Feb 7, 2015

marcysutton Feb 7, 2015

dylanb Feb 7, 2015

marcysutton Feb 7, 2015

marcysutton commented Feb 7, 2015

marcysutton commented Feb 16, 2015

gkalpak Feb 16, 2015

marcysutton Feb 17, 2015

petebacondarwin commented Feb 18, 2015

andrewaustin commented Feb 24, 2015

ginader commented Feb 27, 2015

petebacondarwin commented Mar 2, 2015

marcysutton commented Mar 2, 2015

Roles dynamically added with ngAria #10318

Roles dynamically added with ngAria #10318

Conversation

marcysutton commented Dec 4, 2014

googlebot commented Dec 4, 2014

gkalpak commented Dec 4, 2014

marcysutton commented Dec 4, 2014

gkalpak commented Dec 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Dec 5, 2014

gkalpak commented Dec 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Feb 7, 2015

marcysutton commented Feb 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Feb 18, 2015

andrewaustin commented Feb 24, 2015

ginader commented Feb 27, 2015

petebacondarwin commented Mar 2, 2015

marcysutton commented Mar 2, 2015